-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow xml docs to still be created when 'emit metadata only' is on. #57667
Allow xml docs to still be created when 'emit metadata only' is on. #57667
Conversation
@dotnet/roslyn-compiler ptal :) |
…mabadi/roslyn into metadataOnlyEmitXmlDocs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (iteration 7)
a0c3984
to
f6b9a4a
Compare
{ | ||
public static void SayHello() | ||
{ | ||
ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole.WriteLine(""hello""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 10) modulo typo in test
@dotnet/roslyn-compiler for second review. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except the feedback above (just have to address before merge).
xmlDocBytes = xmlStream.ToArray(); | ||
} | ||
|
||
// This should not fail the emit (as it's a warning). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please remove this comment here and the one below // Even though docs failed, ...
Feel free to merge/squash once the typo and last comment are addressed/considered. Thanks |
{ | ||
public static void SayHello() | ||
{ | ||
ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole.WriteLine(""hello""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole.WriteLine(""hello""); | |
EmitMetadataOnly_XmlDocs_NoDocMode_Success.WriteLine(""hello""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right, aren't we trying to call Console.WriteLine here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh...
…mabadi/roslyn into metadataOnlyEmitXmlDocs
* upstream/main: (1035 commits) Add missing header Mark IVSTypeScriptFormattingServiceImplementation as optional, but require it in the constructor Fix Go To Base for a symbol with a single metadata location (dotnet#58965) [EnC] Store entire spans instead of line deltas (dotnet#58831) Delete CodeAnalysisRules.ruleset (dotnet#58968) Allow xml docs to still be created when 'emit metadata only' is on. (dotnet#57667) Fix ParseVBErrorOrWarning (dotnet#47833) Update parameter nullability to match implementation Ensure CSharpUseParameterNullCheckingDiagnosticAnalyzer works with nullable parameters Add tests for issues fixed by previous PR (dotnet#58764) Update src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Disallow null checking discard parameters (dotnet#58952) Add extension method Escape type arguments Few fixes Update tests. Add Analyzers layer to CODEOWNERS Add formatting analyzer test for param nullchecking (dotnet#58936) Move reading HideAdvancedMembers option up (dotnet#58747) List patterns: Slice value is assumed to be never null (dotnet#57457) ...
EmitMetadataOnly is a flag that exists for IDE scenarios to allow it to emit dlls cheaply that give hte top-level surface area of a particular compilation. This can then be used in cross-language scenarios to allow things like intellisense/QI/and-lots-more to work in a live fashion. We call these cross-language metadata-only dlls: 'skeleton assemblies'.
The IDE would like to improve our story around skeleton assemblies by being able to cache them to our persistence store so that later launched sessions of VS can reuse them without recomputing them.
This is generally not difficult for us to do except for one small snag. Specifically, one thing we support today with skeletons is being able to expose xml doc comments through the skeleton. This works by having us actually both emit the metadta-only-dll, but also creating a DocumentationProvider that wraps the original language compilation. So, for example, if VB references C#, it will get a dll for the C# metadata, but it will get a doc-provider that forwards along to the C# compilation to answer questions.
If we want to persist the dll (and still have xml-doc-comments work across languages) then we need to persist the xml doc info as well. This PR updates the compiler to support that. Previously, the compiler would completely skip xml doc generation if EmitMetadataOnly was set. With this PR, the compiler will emit xml doc comments as well if the appropriate stream is provided.
Relates to ref assemblies: #18612